Taste Rating: 🟢 Good taste

This is an excellent fix for a subtle but critical race condition. The implementation is clean, well-documented, and properly tested.

Code Quality Highlights:

• Clear refactoring with _drain_terminal_status_queue() and _immediate_terminal_statuses()
• Excellent inline documentation explaining the WebSocket hint vs REST polling pattern
• Strong regression tests that directly reproduce the race scenario
• Proper handling of ERROR/STUCK as immediate terminal states

Process Note: This PR changes conversation termination behavior (agent behavior that could affect benchmarks). While the PR includes GitHub Actions links to successful ProgramBench runs, the custom review guidelines require either:

1. Links to the eval monitor (openhands-eval-monitor.vercel.app) showing completed benchmarks, and
2. Human maintainer confirmation of the eval results

Since these are GitHub Actions links (not eval monitor) and no human has confirmed the results in the PR, I'm flagging this for human review per the eval-risk policy.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟢 LOW

This is a correctness fix for a real production bug discovered in ProgramBench retry-16. The change makes the system more correct by treating WebSocket FINISHED events as wakeup hints rather than authoritative termination signals. This prevents premature client teardown when stop hooks veto completion.

Risk factors:

• ✅ Well-tested with regression coverage for both the race scenario and ERROR fast-path preservation
• ✅ Changes isolated to RemoteConversation termination logic
• ✅ Solves a real bug (not theoretical)
• ✅ Evidence of successful ProgramBench runs provided
• ⚠️ Could add minor latency to FINISHED termination (now requires REST polls), but this is the correct behavior

VERDICT:
✅ Recommend merging after human review: Core fix is sound and necessary for correct stop-hook behavior. The eval evidence shows ProgramBench now works correctly with this change.

KEY INSIGHT:
The separation of "immediate terminal statuses" (ERROR/STUCK) from "polled terminal statuses" (FINISHED) elegantly captures the semantic difference between non-revertable errors and potentially-vetoed agent completion.

✅ QA Report: PASS

Verified the RemoteConversation FINISHED stop-hook race fix works correctly by exercising the changed code and analyzing the implementation.

Does this PR achieve its stated goal?

Yes. This PR successfully fixes the race condition where WebSocket FINISHED events could cause RemoteConversation to terminate prematurely before server-side stop hooks finished evaluating whether the conversation should actually stop.

The fix correctly implements the behavior described in the PR: WebSocket FINISHED is now treated as a wake-up hint rather than authoritative termination, while ERROR and STUCK remain immediate terminal states. REST polling with a threshold of 3 consecutive terminal polls provides authoritative confirmation before the client considers the run complete.

uv run python qa_simple_verification.py

Test 1: FINISHED via WebSocket is a hint, not authoritative termination
----------------------------------------------------------------------
Immediate terminal statuses: frozenset({'error', 'stuck'})
✅ PASS: FINISHED is NOT in immediate terminal statuses
✅ PASS: ERROR and STUCK are immediate terminal statuses

Testing _drain_terminal_status_queue behavior...
Queue size before drain: 3
Queue size after drain: 0
✅ PASS: Queue draining correctly empties all items

Test 2: ERROR and STUCK terminate immediately (not hints)
----------------------------------------------------------------------
ERROR is_terminal: True
✅ PASS: ERROR is a terminal status
STUCK is_terminal: True
✅ PASS: STUCK is a terminal status
FINISHED is_terminal: True
✅ PASS: FINISHED is a terminal status (but treated as hint in WS path)

Test 3: REST polling confirmation pattern
----------------------------------------------------------------------
✅ PASS: TERMINAL_POLL_THRESHOLD is set to 3 in _wait_for_run_completion
✅ PASS: REST polling requires TERMINAL_POLL_THRESHOLD consecutive polls
   This prevents premature termination when stop hooks revert FINISHED → RUNNING
✅ PASS: Counter resets when status flips back to non-terminal (e.g., FINISHED → RUNNING)
   This correctly handles stop hook denials

gh pr view 3191 --repo OpenHands/software-agent-sdk --json statusCheckRollup,state

Issues Found

None.

Summary: The fix correctly addresses the stop-hook race condition by treating WebSocket FINISHED as a wakeup hint rather than authoritative termination. REST polling with a 3-poll threshold provides reliable confirmation that the server has actually finished (and stop hooks have been evaluated). The implementation is clean, well-tested, and preserves the fast-path for ERROR/STUCK states.

Taste Rating: 🟡 Acceptable

Found one important regression-test gap: the new test does not actually inject the WS FINISHED event after run() drains stale queue items, so it would not fail against the old behavior. Also leaving this as COMMENT (not approval) because the PR changes run-loop behavior and should get maintainer/eval sign-off.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟡 MEDIUM — client/server run completion semantics affect benchmark/runtime correctness, but the implementation direction is straightforward.

This review was generated by an AI agent (OpenHands) on behalf of the user.

Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/OpenHands/software-agent-sdk/actions/runs/26368765094

Taste Rating: 🟡 Acceptable, but there is still a completion race to fix. One concrete non-inline issue: the final refresh_from_server() result is currently not enforced; if it returns running, _handle_conversation_status() returns False but _wait_for_run_completion() still reconciles and returns.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟡 MEDIUM — Remote run-completion semantics affect eval/runtime correctness; this should stay COMMENT until the remaining race is fixed and maintainer/eval sign-off happens.

Automated review generated by OpenHands on behalf of the user.

Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/OpenHands/software-agent-sdk/actions/runs/26369466261

Taste Rating: 🟡 Acceptable, but there is still one terminal-status race to fix.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟡 MEDIUM — Remote run-completion semantics affect runtime/eval correctness, and this changes agent behavior, so this remains COMMENT for maintainer/eval sign-off.

VERDICT: ❌ Needs rework on the active-run ERROR/STUCK drain race.

This review was generated by an AI agent (OpenHands) on behalf of the user.

Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/OpenHands/software-agent-sdk/actions/runs/26369803053

The fix is technically correct and the root cause analysis is thorough. Treating WebSocket FINISHED as advisory and requiring the post-run full-state snapshot as the authoritative completion signal cleanly closes the stop-hook race. The _run_armed gate preventing pre-run subscription snapshots from poisoning a future run() call is a nice touch. Two targeted comments below.

This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation

✅ QA Report: PASS

Verified the RemoteConversation FINISHED/stop-hook race behavior with actual SDK RemoteConversation.run() calls against a live local HTTP agent-server simulator; the PR fixes the early-return behavior and preserves ERROR fast-path handling.

Does this PR achieve its stated goal?

Yes. On origin/main, the same simulated server ordering caused run() to return after a WebSocket FINISHED even though the authoritative REST state had already reverted to running, reproducing the stop-hook race. On this PR, run() ignored the per-field FINISHED, observed the running veto window, and returned only after the post-run full-state snapshot; ERROR still raised immediately without polling.

Run completed via WebSocket notification (status: finished, elapsed: 0.1s)
{"elapsed_ticks_seconds": 0.15, "error": "", "outcome": "returned", "poll_count": 1, "posts_to_run": 1, "scenario": "finished_race", "seen_statuses": ["running"]}

Run completed via post-run WebSocket state update (status: finished, elapsed: 0.4s)
{"elapsed_ticks_seconds": 0.5, "error": "", "outcome": "returned", "poll_count": 5, "posts_to_run": 1, "scenario": "finished_race", "seen_statuses": ["running", "running", "running", "finished", "finished"]}

{"elapsed_ticks_seconds": 0.15, "error": "ConversationRunError: Conversation run failed for id=9b21f108-8f7-4aa1-89bf-b6fea6dd241e: Remote conversation ended with error", "outcome": "raised", "poll_count": 0, "posts_to_run": 1, "scenario": "error_fast_path", "seen_statuses": []}

{"elapsed_ticks_seconds": 0.15, "error": "ConversationRunError: Conversation run failed for id=f8bf31b9-f806-483c-b0b8-d1eb4ebe4c40: Remote conversation ended with error", "outcome": "raised", "poll_count": 0, "posts_to_run": 1, "scenario": "error_fast_path", "seen_statuses": []}

Issues Found

None.

This review was created by an AI agent (OpenHands) on behalf of the user.

Review: Fix RemoteConversation FINISHED stop-hook race

Summary

This is a well-designed and carefully-implemented fix for a subtle but genuine race condition. The root cause is clear: the server-side LocalConversation.run loop releases its state lock at the end of each iteration, so a FINISHED status set by agent.step() is observable to WebSocket clients before stop hooks run in the next iteration. Stop hooks can flip FINISHED back to RUNNING, making the WS-delivered FINISHED advisory rather than authoritative.

Correctness

The fix is correct. The key insight — treating WebSocket FINISHED as a hint and waiting for the server's post-run full-state snapshot as the authoritative termination signal — properly closes the race without degrading the ERROR/STUCK fast path. The _IMMEDIATE_TERMINAL_STATUSES set and _RunCompletionSignal.from_post_run_snapshot flag make the two-track behavior explicit and type-safe.

The _run_armed guard is elegant: clearing before the POST and setting after it ensures the initial WS subscription snapshot (delivered during connect()) can never be mistaken for the post-run completion signal. The finally: self._run_armed.clear() cleanup means the guard is always restored regardless of how _wait_for_run_completion exits.

REST Hard Fallback

The 30-second time-based hard fallback (TERMINAL_HARD_FALLBACK_SECS) is a sensible safety valve for the case where the WebSocket drops after the server finishes but before the post-run full-state snapshot is delivered. Hardcoding 30 s is reasonable given the stated rationale (stop hooks are fast; 30 s exceeds any reasonable hook execution time).

One nuance: the terminal_first_seen_at timer resets on every REST poll exception (see inline comment). In a degenerate scenario where the server alternates between reporting terminal status and throwing HTTP errors, the 30 s threshold could keep sliding indefinitely with timeout=None. Noted inline for awareness — not a blocker.

Test Coverage

The regression suite is thorough:

• test_remote_conversation_run_waits_for_post_run_full_state — verifies the client does not return on the first REST-visible FINISHED without a WS snapshot.
• test_remote_conversation_run_stale_pre_run_snapshot_is_ignored — verifies _run_armed rejects pre-run WS snapshots.
• test_remote_conversation_run_ws_error_still_terminates_immediately — confirms ERROR short-circuits without waiting for REST.
• test_remote_conversation_run_rest_hard_fallback_when_ws_silent — exercises the 30 s fallback via time.monotonic patching.
• test_remote_conversation_run_keeps_error_queued_after_running_poll — confirms ERROR is not discarded by a racing non-terminal REST poll.

The install_post_run_full_state helper is well-documented (the comment explaining why it fires from GET rather than POST — to avoid racing _run_armed.set() — is particularly valuable).

Non-Core Changes

The programbench additions across skill files, the workflow, and manage_evals.py are clean mechanical updates. The AGENTS.md <KNOWN_RACES_AND_GOTCHAS> section is high-quality institutional knowledge that will save the next developer who encounters anything adjacent to this race.

Verdict

✅ The fix is correct, the tests are solid, and the documentation is exemplary. No blocking concerns.

This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation

✅ QA Report: PASS

RemoteConversation now waits through a simulated FINISHED→RUNNING stop-hook race and completes only after the authoritative post-run full-state snapshot.

Does this PR achieve its stated goal?

Yes. I reproduced the pre-fix behavior on origin/main: RemoteConversation.run(blocking=True) returned from a WebSocket execution_status=finished hint even though the simulator's next authoritative REST status was running, matching the stop-hook race described in the PR. On this PR, the same user-facing RemoteConversation run ignored that FINISHED hint, observed three running polls, then returned only after the post-run full-state snapshot; I also verified WebSocket ERROR still raises immediately without REST polling.

cd /tmp/sdk-qa-main && OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_remote_finished_race.py --scenario race

Run completed via WebSocket notification (status: finished, elapsed: 0.0s)
SCENARIO=race
OUTCOME=returned
ELAPSED_SECS=0.01
REST_STATUS_POLLS=1
RUNNING_REVERT_SEEN=True
POST_RUN_FULL_STATE_SENT=False
HTTP_SEQUENCE=...,POST /api/conversations/.../run,GET /api/conversations/...

OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_remote_finished_race.py --scenario race

Run completed via post-run WebSocket state update (status: finished, elapsed: 0.2s)
SCENARIO=race
OUTCOME=returned
ELAPSED_SECS=0.22
REST_STATUS_POLLS=4
RUNNING_REVERT_SEEN=True
POST_RUN_FULL_STATE_SENT=True
HTTP_SEQUENCE=...,POST /api/conversations/.../run,GET /api/conversations/...,GET /api/conversations/...,GET /api/conversations/...,GET /api/conversations/...,GET /api/conversations/.../events/search

OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_remote_finished_race.py --scenario ws-error

SCENARIO=ws-error
OUTCOME=raised
ERROR=ConversationRunError: Conversation run failed for id=...: Remote conversation ended with error
ELAPSED_SECS=0.02
REST_STATUS_POLLS=0
POST_RUN_FULL_STATE_SENT=False

Issues Found

None.

This QA review was created by an AI agent (OpenHands) on behalf of the user.

Taste Rating: 🟢 Good taste

This is a well-executed fix for a real and subtle race condition. The PR has been through multiple review iterations and the current HEAD (b5bec8c) is in clean shape.

Root cause & fix correctness

The race is real: the server-side LocalConversation.run loop releases its state lock at the end of each iteration, so a FINISHED status set by agent.step() is observable by WS clients before the next iteration's stop hooks run. A stop hook returning rc=2 flips the status back to RUNNING — but the old client already exited. The fix correctly identifies the authoritative signal: not the per-field WS FINISHED event, but the server's post-run full-state ConversationStateUpdateEvent published after conversation.run()/arun() exits and pending events flush.

Three-part termination design

1. _IMMEDIATE_TERMINAL_STATUSES (ERROR, STUCK) — per-field WS updates are still immediately terminal for these; stop hooks cannot revert them.
2. Per-field WS FINISHED — excluded from _IMMEDIATE_TERMINAL_STATUSES; treated as advisory only and never enqueued for the fast-path.
3. Post-run full-state WS snapshot — authoritative; gated by _run_armed (set after POST /run, cleared in finally) to exclude the initial subscription snapshot that arrives during connect().

This is exactly the right separation of concerns.

Invariant guard at line 1153

The final iteration's fix — replacing assert signal.from_post_run_snapshot with an explicit raise AssertionError(...) — is correct. The assert form is silenced by python -O and some packaging tools; raise AssertionError(...) is not. The guard is currently theoretically unreachable (ERROR/STUCK raise in _handle_conversation_status before reaching this line, so only from_post_run_snapshot=True signals arrive), but documents the invariant and will catch regressions if _IMMEDIATE_TERMINAL_STATUSES is extended with a status that doesn't raise.

Hard fallback

TERMINAL_HARD_FALLBACK_SECS = 30.0 is the right safety valve for WS delivery failures (dropped socket after run finishes on the server). The timer resets on poll errors, which correctly prefers false-negative waits over false-positive early returns. 30 s is a generous but reasonable bound — stop hooks are fast; a hook revert would flip REST back to RUNNING within milliseconds, well before the threshold.

Tests

The new test cases exercise real code paths and assert on outputs:

• test_remote_conversation_run_ws_finished_is_only_a_hint_not_terminal — the core regression: WS-delivered FINISHED after /run trigger does not complete run().
• test_remote_conversation_run_rest_finished_revert_waits_for_full_state — REST FINISHED→RUNNING revert does not prematurely return.
• test_remote_conversation_run_stale_pre_run_snapshot_is_ignored — _run_armed guard works end-to-end.
• test_remote_conversation_run_ws_error_still_terminates_immediately — ERROR via WS still fast-paths (poll_interval=10s would make a fall-through take 30+ s).
• test_remote_conversation_run_rest_hard_fallback_when_ws_silent — hard fallback activates when WS snapshot is never delivered.
• install_post_run_full_state helper fires the WS event from the GET side effect (after _run_armed.set()) rather than from the POST side effect, correctly avoiding the arming race.

Evidence

ProgramBench CI links are present and passing (parent eval job, downstream inference phase, downstream eval harness phase).

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟡 MEDIUM — Remote run-completion semantics are core infrastructure. The change is narrowly targeted, all previously raised concerns are resolved in the current HEAD, and regression test coverage is solid. The 30 s hard fallback prevents indefinite hangs if WS delivery fails. Recommend merging after final maintainer sign-off.

VERDICT:
✅ Worth merging: The fix is correct, the invariant guard is robust, tests are real, and the evidence is present. All concerns from previous review rounds are resolved.

KEY INSIGHT:
Per-field WS FINISHED is advisory; only the server's post-run full-state snapshot is authoritative, because stop hooks can revert FINISHED before the server's run loop exits.

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing. See the customization docs for the required frontmatter format.
2. Re-request a review — the reviewer reads guidelines from the PR branch, so your changes take effect immediately.

Was this review helpful? React with 👍 or 👎 to give feedback.

This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation

✅ QA Report: PASS

The RemoteConversation race fix was functionally verified with a real SDK client talking to a local HTTP/WebSocket server that reproduced the FINISHED stop-hook race.

Does this PR achieve its stated goal?

Yes. On origin/main, RemoteConversation.run(blocking=True) returned immediately after a per-field WebSocket FINISHED hint while REST still reported running, reproducing the premature-return bug. On PR commit b5bec8c7, the same scenario waited through the reverted running statuses and returned only after the post-run full-state WebSocket snapshot reported finished; a separate run also confirmed WebSocket ERROR still raises immediately.

{
  "log": "Run completed via WebSocket notification (status: finished, elapsed: 0.0s)",
  "run_returned": true,
  "elapsed_seconds": 0.003,
  "run_get_count": 1,
  "statuses_returned": ["running"],
  "final_cached_status": "running"
}

{
  "log": "Run completed via post-run WebSocket state update (status: finished, elapsed: 0.2s)",
  "run_returned": true,
  "elapsed_seconds": 0.211,
  "run_get_count": 4,
  "statuses_returned": ["running", "running", "running", "finished"],
  "final_cached_status": "finished"
}

{
  "raised": "ConversationRunError",
  "elapsed_seconds": 0.004,
  "run_get_count": 0,
  "message_contains_error": true
}

Issues Found

None.

This QA review was generated by an AI agent (OpenHands) on behalf of the user.

⚠️ QA Report: PASS WITH ISSUES

The FINISHED stop-hook race is fixed in the exercised RemoteConversation path, but QA found a related regression in immediate WebSocket ERROR handling.

Does this PR achieve its stated goal?

Yes for the primary goal. Using a short SDK harness that calls RemoteConversation.run() against a fake remote server, the base branch returned after a per-field WebSocket FINISHED while the server was still reporting RUNNING; the PR ignored that hint and waited until the post-run full-state snapshot arrived. However, the same exercise shows per-field WebSocket ERROR is no longer surfaced immediately, despite the PR description saying ERROR/STUCK remain immediate terminal states.

5e614afe
Run completed via WebSocket notification (status: finished, elapsed: 0.0s)
finished_race_rest_polls=1

6808b8a8
REST has reported terminal status finished for 3 polls without a post-run WebSocket snapshot; continuing to wait for the authoritative snapshot (elapsed: 0.1s)
Run completed via post-run WebSocket state update (status: finished, elapsed: 0.1s)
finished_race_rest_polls=6

per_field_error_result=ConversationRunError after 0.00s; REST polls=0; message=Conversation run failed for id=2d30afb2-ff95-46e8-a902-a14442d222d9: Remote conversation ended with error

per_field_error_result=ConversationRunError after 0.26s; REST polls=5; message=Conversation run failed for id=b835439a-8ef7-4acf-954e-c109e1980e76: Run timed out after 0.25 seconds. The conversation may still be running on the server.

Issues Found

• 🟠 Issue: Per-field WebSocket ERROR/STUCK updates no longer appear to be immediate terminal signals; a user can now wait until polling/timeout if REST remains stale.

This review was created by an AI agent (OpenHands) on behalf of the user.

Taste Rating: 🟢 Good taste

This is a well-executed fix for a real, production-observed race condition. The root cause analysis is accurate: the server-side LocalConversation.run loop releases its state lock at iteration end, so a per-field FINISHED WS event can arrive before stop hooks run in the next iteration. The architectural fix — treating per-field status updates as advisory and requiring the post-run full-state WS snapshot as the authoritative completion signal — is the correct approach.

The _run_armed (threading.Event) mechanism elegantly prevents the initial WS subscription snapshot from being mistaken for a post-run completion signal by gating the callback before the POST /run call. The 30-second time-based TERMINAL_HARD_FALLBACK_SECS is a well-reasoned safety valve: stop hooks complete in seconds, so 30 s provides a strong signal that the WS snapshot was lost without introducing a painful hang. Tests are comprehensive, exercise real code paths, and include a patch of time.monotonic to deterministically trigger the hard-fallback path — well above the bar for regression coverage.

[IMPROVEMENT OPPORTUNITIES]

AGENTS.md note references non-existent symbols

The newly added <KNOWN_RACES_AND_GOTCHAS> note says "the REST poll path with TERMINAL_POLL_THRESHOLD consecutive terminal polls" and "See _immediate_terminal_statuses". Neither symbol exists in the current implementation:

• The count-based TERMINAL_POLL_THRESHOLD was replaced by the time-based TERMINAL_HARD_FALLBACK_SECS = 30.0.
• _immediate_terminal_statuses is not a method on RemoteConversation; ERROR/STUCK are handled inline inside _handle_conversation_status.

A future contributor reading AGENTS.md and then grepping the codebase for these symbols will find nothing. Suggest updating the sentence to reference TERMINAL_HARD_FALLBACK_SECS and _handle_conversation_status.

_handle_conversation_status return value is never consumed

The method signature is -> bool and its docstring says "return True if the run is complete." However, both call sites (WS path and REST path in _wait_for_run_completion) discard the return value. Callers rely entirely on the exception-raise side effect for ERROR/STUCK. Either drop the return type to None, or add a callsite that consumes it. As written it creates a false impression that the bool controls downstream logic.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟢 LOW — the change tightens run-completion semantics and is covered by 32 passing tests plus end-to-end ProgramBench CI evidence cited in the PR description. The eval-infrastructure changes (adding programbench to benchmark lists) are purely additive. The only rollback risk is if a deployment environment consistently drops the post-run WS full-state snapshot, in which case the 30 s hard fallback adds latency; that scenario is preferable to the previous premature-termination bug.

VERDICT:
✅ Worth merging: Core logic is sound, tests are solid, real production evidence provided. The AGENTS.md stale references and unused return type are polish items, not blockers.

This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better by adding a .agents/skills/custom-codereview-guide.md file to your branch with the /codereview trigger and context the reviewer is missing.

Was this review helpful? React with 👍 or 👎 to give feedback.

✅ QA Report: PASS

Verified the RemoteConversation stop-hook race behavior with a functional SDK harness: the PR no longer returns on an early WebSocket FINISHED hint and still raises immediately on WebSocket ERROR.

Does this PR achieve its stated goal?

Yes. The stated goal is to prevent RemoteConversation.run(blocking=True) from treating a per-field WebSocket FINISHED update as authoritative when a server-side stop hook may revert the run to RUNNING. In the baseline run, the client returned immediately after the early WS FINISHED and before the simulated post-run full-state snapshot (delivered_full_state: false). With the PR applied, the same user-facing RemoteConversation.run(...) call stayed active through the simulated stop-hook veto window and returned only after the authoritative full-state snapshot (poll_count: 5, delivered_full_state: true).

{
  "outcome": "returned",
  "error": null,
  "poll_count": 1,
  "delivered_full_state": false,
  "elapsed_seconds": 0.003
}

Run completed via WebSocket notification (status: finished, elapsed: 0.0s)

{
  "outcome": "returned",
  "error": null,
  "poll_count": 5,
  "delivered_full_state": true,
  "elapsed_seconds": 0.26
}

Run completed via post-run WebSocket state update (status: finished, elapsed: 0.3s)

{
  "outcome": "raised",
  "error": "ConversationRunError: Conversation run failed for id=...: qa injected error",
  "poll_count": 0,
  "elapsed_seconds": 0.004
}

Issues Found

None.

This review was created by an AI agent (OpenHands) on behalf of the user.

Solid fix for a subtle but real race. The core design — treating per-field WS FINISHED as advisory and only accepting full-state snapshots as authoritative completion signals — is the right call, and the _run_armed gate elegantly prevents the initial subscription snapshot from masquerading as a post-run signal. Test coverage is thorough, including the regression scenario, ERROR short-circuit, hard-fallback path, and stale-snapshot guard.

Two minor observations below; neither is a blocker.

This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation